Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove libcurl dependency #9

Merged

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan commented Apr 25, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

Unix builds need not depends on libcurl:

Also libcurl properly encodes runtime requirements, so its presence in the run section is not required.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jjerphan jjerphan force-pushed the only_make_windows_depends_on_libcurl branch from eaa86ff to 56fedf4 Compare April 25, 2024 09:32
@jjerphan jjerphan changed the title Only make Windows builds depends on libcurl Only make Windows builds depend on libcurl Apr 25, 2024
Copy link
Contributor

@pidefrem pidefrem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @jjerphan. The condition for the unix build dependency on libcurl is:

 if( NOT USE_SYSTEM_TZ_DB AND NOT MANUAL_TZ_DB )

In the build.sh file,

cmake -DCMAKE_INSTALL_PREFIX=$PREFIX $SRC_DIR -DCMAKE_INSTALL_LIBDIR=lib -DBUILD_SHARED_LIBS=ON -DUSE_SYSTEM_TZ_DB=ON -DBUILD_TZ_LIB=ON -DCMAKE_CXX_FLAGS="-fPIC" -DCMAKE_BUILD_TYPE=Release $CMAKE_CXX_FLAGS

the DUSE_SYSTEM_TZ_DB is indeed set to ON.
The option is ON as well for the windows build.

How do you justify keeping the dependency for windows if you remove it for unix?

@jjerphan
Copy link
Member Author

Hi @pidefrem

Thank you for making this observation.

The dependency might also be removed for Windows, indeed.

To be fair with you, I have not looked into that further as the project I work only depends on this library on unix (effectively when it is built against libcpp).

I am currently off, but I may have a look then. Feel free to modify this PR or merge it.

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@jjerphan jjerphan requested a review from pidefrem May 14, 2024 14:39
@pidefrem pidefrem changed the title Only make Windows builds depend on libcurl Remove libcurl dependency May 14, 2024
@jjerphan jjerphan added the automerge Merge the PR when CI passes label May 14, 2024
@jjerphan jjerphan merged commit 7a30c43 into conda-forge:main May 14, 2024
6 of 7 checks passed
@jjerphan jjerphan deleted the only_make_windows_depends_on_libcurl branch May 14, 2024 16:46
@jjerphan
Copy link
Member Author

I believe the dependency on libcurl for Windows was meant to use the network due to HowardHinnant/date#788 and similar other issues.

@jjerphan
Copy link
Member Author

Actually HowardHinnant/date#611.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge the PR when CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants